Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ad/persistent menu mouseover #35566

Merged
merged 39 commits into from
Jan 10, 2025
Merged

Conversation

Robert-Costello
Copy link
Contributor

@Robert-Costello Robert-Costello commented Dec 30, 2024

Product Description

This makes it so that the persistent nav sidebar pops open as an overlay when the user hovers over the collapsed menu.
The menu can be locked open which will do two things.
1: it will fit the nav menu inline with the other elements on the page (no longer an overlay).
2: the nav menu will continue to be expanded as the user navigates between menus.
mouseover

Technical Summary

Jira ticket for mouseover
In order to have have a transition for the expanding and collapsing you need to know the width the menu will be when expanded. To accomplish this setPersistantMenuRegionWidth sets up invisible clones of the elements that make up the nav menu and stores that value in sessionStorage so that it only needs to be calculated once per session. The value, persistantMenuRegionWidth, is reset when the page is initialized.
I looked at the possibility of using a css calc to determine the width, but I don't think that will work. The width will depend on the text content of the menu and the elements need to be appended to the DOM in order to get the correct width.
I'm pretty happy with the current approach, but open to other suggestions.

Jira ticket for defaulting nav menu to closed when split screen case search is on.

Feature Flag

persistent_menu_setting

Safety Assurance

Safety story

tested locally and on staging

changes are limited to this feature flag and the only risk I can see is undesirable ui on the persistent men nav bar.

Automated test coverage

QA Plan

no QA planned

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@dimagimon dimagimon added the Risk: Medium Change affects files that have been flagged as medium risk. label Dec 30, 2024
@Robert-Costello Robert-Costello requested review from a team, MartinRiese, AddisonDunn and minhaminha and removed request for a team December 31, 2024 18:08
@Robert-Costello Robert-Costello marked this pull request as ready for review December 31, 2024 18:08
Copy link
Contributor

@MartinRiese MartinRiese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other thing I noticed in the screen shot is that the content of the menu wraps while it is shrinking. I think it would look better if the number of lines does not change during the transition. But I am not sure how hard that would be.

@Robert-Costello
Copy link
Contributor Author

Robert-Costello commented Dec 31, 2024

I think it would look better if the number of lines does not change during the transition. But I am not sure how hard that would be.

I agree with this. I'm not sure about how hard it would be either, but I'll give it a go.

...turns out it was incredibly easy 5c682d3

Copy link
Contributor

@Jtang-1 Jtang-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice change, the animation is great! Nit if it's easy to implement - what do you thinking about not having the "opening" animation during navigation if the sidebar is already persistently opened?

@Robert-Costello
Copy link
Contributor Author

Robert-Costello commented Jan 2, 2025

Very nice change, the animation is great! Nit if it's easy to implement - what do you thinking about not having the "opening" animation during navigation if the sidebar is already persistently opened?

Thanks! I think that's a good idea. I'll take a stab at it. I don't think it'll be too big of a lift.

...changed here 0346101

Copy link
Contributor

@Jtang-1 Jtang-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a couple comments

const contentPlusContainer = $('#content-plus-persistent-menu-container');

const persistentMenuRegionClone = $('#persistent-menu-region').clone();
persistentMenuRegionClone.attr("id","pmr-clone");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the purpose of adding these ids?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

originally the ids were intended to make sure that there weren't cloned elements floating around that would be difficult to search for/trouble shoot. The clones are being removed immediately, so I can see the argument that giving them ids isn't strictly necessary at this point. I'm on the fence about whether it's better to skip adding the ids. I could be swayed either way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's no use for it now, my preference would be to delete it to remove the chance of confusion of what its purpose is.

Copy link
Contributor

@Jtang-1 Jtang-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes make sense to me. Making the clone to get the width seems a bit hacky but I don't know of a better way to do it. Would be nice to get thoughts from someone more adept at frontend

@orangejenny
Copy link
Contributor

Making the clone to get the width seems a bit hacky but I don't know of a better way to do it.

FWIW, I think this is a normal/appropriate level of hackiness.

orangejenny and others added 4 commits January 9, 2025 12:48
…bs and persistent menu nav should work together while both are available.
…to avoid doubled border. otherwise there is a right border and no top border which looks strange
@Robert-Costello Robert-Costello merged commit b952a5c into master Jan 10, 2025
12 checks passed
@Robert-Costello Robert-Costello deleted the ad/persistent-menu-mouseover branch January 10, 2025 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Risk: Medium Change affects files that have been flagged as medium risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants